Add support for ART Local tests in the APIs #419

zhaoyuyoung · 2024-12-13T19:21:23Z

No description provided.

tkorchug

I will comment line by line, here is a few general comments mostly related to style:

please move any non-view functions from views.py to utils.py (i.e. get_client_ip)
you have misaligned comments e.g. lines 1166, 1169 it is harder to read code, please tab them to align with the next lines
I think the test_type for good jobs should be also set to "grid", right now it will be "None". And related to this, there should be an extra param in query in all existing views query['test_type'] = "grid", so bigpanda takes only grid tests from the DB by default for the transition period. I think setupView() function in art/utils.py is the perfect place to add it

tkorchug · 2024-12-16T12:57:50Z

core/art/views.py


    # log all the req params for debug
    _logger.debug('[ART] registerARTtest requestParams: ' + str(request.session['requestParams']))

    # Checking whether params were provided
+    if 'test_type' in request.session['requestParams']:
+        test_type = request.session['requestParams']['test_type']
+


here should be else: test_type = "grid", otherwise it end up as None. I think the "grid" should be default, at least it is needed before art expert adds this param to the art code.

Made "test_type" default to be "grid".

tkorchug · 2024-12-16T12:59:33Z

core/art/views.py

    if 'requestParams' in request.session and 'pandaid' in request.session['requestParams'] and 'testname' in request.session['requestParams']:
        pandaid = request.session['requestParams']['pandaid']
        testname = request.session['requestParams']['testname']
+    elif test_type == 'local':
+        '''


it is python doc string, which is used in the beginning of each function. I the middle of the code it is better to use comments with "#"

tkorchug · 2024-12-16T13:01:04Z

core/art/views.py

+        # If reverse DNS lookup fails, return the IP address as fallback
+            art_host = client_ip
+
+        if art_host.split('.')[0] in art_const.AUTHORIZED_HOSTS: 


use of pass can be simplified by inverting the if statement, e.g.
if art_host.split('.')[0] not in art_const.AUTHORIZED_HOSTS:
return JsonResponse({"error": "Invalid ART API user!"}, status=403)

Removed "pass". (There were other contents here for test. I forgot to remove it in the first cleaning.).

tkorchug · 2024-12-16T13:08:40Z

core/art/views.py

+
+        # Generate job ID for ART Local
+        query = {'test_type': 'local'}
+        localIDs = []


Here no need to get all pandaids from DB, it is sufficient to use aggregate functionality of Django ORM, there is an example of in remove_old_tests() view, it should be around line 1622.

Changed to the Django ORM aggregration.

tkorchug · 2024-12-16T13:13:24Z

core/art/views.py

+        else:
+            pandaid = art_const.INITIAL_LOCAL_ID
+        _logger.info("JobID: {} was generated".format(pandaid))
+        testname = request.session['requestParams']['testname']


this line is the same for both local and grid jobs, and it is duplicated with 1158, it is better to move it out of if statement.

Now both local and grid have the commond code.

tkorchug · 2024-12-16T13:17:58Z

core/art/views.py

-    if 'jobname' in job:
+    # Only check ART Grid tests
+    if test_type and test_type == 'local':
+        branch = concat_branch({'nightly_release_short':nightly_release_short, 'project': project, 'platform': platform})


the same as testname, no need to have the same lines for both types of tests inside if. Please move it outside to avoid duplication.

Both the local and grid tests have the same code for this too.

tkorchug · 2024-12-16T13:19:33Z

core/art/views.py

+    # Only check ART Grid tests
+    if test_type and test_type == 'local':
+        branch = concat_branch({'nightly_release_short':nightly_release_short, 'project': project, 'platform': platform})
+        attemptnr = 1


this can be moved up to around 1187

tkorchug · 2024-12-16T13:21:03Z

core/art/views.py

+    if test_type and test_type == 'local':
+        branch = concat_branch({'nightly_release_short':nightly_release_short, 'project': project, 'platform': platform})
+        attemptnr = 1
+    else:


and here it can be simple if test_type == "grid", instead of if-else, after the previous two lines moved to their places

Since you have added test_type == "grid" to the existing jobs in the ART tables, this has been changed as suggested.

tkorchug · 2024-12-16T13:22:30Z

core/art/views.py

@@ -1265,6 +1316,8 @@ def registerARTTest(request):
        _logger.exception('Failed to parse date from nightly_tag')

    # Check whether the pandaid has been registered already
+    if test_type and test_type == 'local':
+        computingsite = "ART Local"


this can be also moved up to ~1187, to shorten code and avoid this if statement.

Move as suggested.

tkorchug · 2024-12-16T13:25:30Z

core/art/views.py

            )
            insertRow.save()
-            data = {'exit_code': 0, 'message': "Provided pandaid has been successfully registered"}
+            data = {'exit_code': 0, 'PandaID': pandaid, 'message': "Provided pandaid has been successfully registered"}


I do not see why to used capital letters here, "pandaid" should be OK. And the message may be updated, something like "Test has been registered" or something like that

Using pandaid in lower case now.

zhaoyuyoung · 2024-12-16T14:20:01Z

Thanks Tanya for reviewing the messay codes! I cleaned them quickly but didn't take care of much details Friday. I'll address each of your comments and commit again. About the third general comment:

- I think the test_type for good jobs should be also set to "grid", right now it will be "None". And related to this, there should be an extra param in query in all existing views query['test_type'] = "grid", so bigpanda takes only grid tests from the DB by default for the transition period. I think setupView() function in art/utils.py is the perfect place to add it This is a good solution for the test and not showing the local records in

the dev instance. Because ADCR will be used for the test, the local results will be part of it, I think the production bigpanda will still see them. Am I wrong? (I've thought about this over the weekend and plan to ask you about it.) Also, if require query['test_type'] = "grid", it's good for the dev bigpanda only rendering the webpages for ART Grid jobs. In the production case, when merged, local and grid ART are rendered the same way (very few places need to take special care for the local jobs, if we will use the same display style). Best, Zhaoyu

…

On Mon, Dec 16, 2024 at 7:55 AM Tatiana Korchuganova < ***@***.***> wrote: ***@***.**** commented on this pull request. I will comment line by line, here is a few general comments mostly related to style: - please move any non-view functions from views.py to utils.py (i.e. get_client_ip) - you have misaligned comments e.g. lines 1166, 1169 it is harder to read code, please tab them to align with the next lines - I think the test_type for good jobs should be also set to "grid", right now it will be "None". And related to this, there should be an extra param in query in all existing views query['test_type'] = "grid", so bigpanda takes only grid tests from the DB by default for the transition period. I think setupView() function in art/utils.py is the perfect place to add it — Reply to this email directly, view it on GitHub <#419 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYRK4OJ2Z3IRCLBLTGFQG32F3ESVAVCNFSM6AAAAABTSS4BTGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBWGEYTMOJSGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

zhaoyuyoung · 2024-12-27T18:01:45Z

Thanks for your reviews! I've accomodated your suggestions to the new commit.
Since you have added "test_type" to all existing jobs in the ART tables, test_type="grid" is the default query now. This should help only display ART grid tests in the bigpanda.

tkorchug reviewed Dec 16, 2024

View reviewed changes

Add support for ART Local tests in the APIs

39b57ed

zhaoyuyoung force-pushed the zyang_dev branch from 9006817 to 39b57ed Compare December 27, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ART Local tests in the APIs #419

Add support for ART Local tests in the APIs #419

zhaoyuyoung commented Dec 13, 2024

tkorchug left a comment

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

tkorchug Dec 16, 2024

zhaoyuyoung Dec 27, 2024

zhaoyuyoung commented Dec 16, 2024 via email

zhaoyuyoung commented Dec 27, 2024

Add support for ART Local tests in the APIs #419

Are you sure you want to change the base?

Add support for ART Local tests in the APIs #419

Conversation

zhaoyuyoung commented Dec 13, 2024

tkorchug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyuyoung commented Dec 16, 2024 via email

zhaoyuyoung commented Dec 27, 2024